-
Notifications
You must be signed in to change notification settings - Fork 302
feat: capture raw body bytes for v4 hmac calculation #8065
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@claude please review this PR |
ad4d7cf to
3c59ffe
Compare
a7dbd99 to
35e2582
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR aims to preserve exact incoming HTTP request body bytes in the Express app so downstream v4 HMAC/signature verification can use the unmodified payload, and to forward that raw payload unchanged when proxying requests.
Changes:
- Capture and store the raw JSON request body buffer during parsing (
req.rawBodyBuffer). - Add a helper to prefer forwarding the raw body when proxying (
sendRequestBody) and apply it across body-bearing HTTP methods. - Add unit tests validating raw-body capture behavior (whitespace/order preservation).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
modules/express/src/expressApp.ts |
Adds bodyParser.json verify hook to capture raw request bytes into req.rawBodyBuffer. |
modules/express/src/clientRoutes.ts |
Introduces sendRequestBody and updates redirectRequest to forward raw bytes when present. |
modules/express/types/express/index.d.ts |
Extends Express Request typing with optional rawBodyBuffer. |
modules/express/test/unit/clientRoutes/rawBodyBuffer.ts |
Adds tests validating raw-body buffer capture and preservation characteristics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude review this PR with a primary focus on the bug, and confirm whether these changes impact any other flows. |
35e2582 to
fb72755
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b9c19c2 to
03e3bce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
03e3bce to
1c0014f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1c0014f to
f5afe26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude review this PR with a primary focus on the bug, and confirm whether these changes impact any other flows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f5afe26 to
0a89476
Compare
0a89476 to
b47123c
Compare
b47123c to
2019b7a
Compare
pranavjain97
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
TICKET: CAAS-659
This pull request improves how HTTP request bodies are handled in the Express app, specifically to support v4 HMAC authentication that requires the exact raw request bytes. The main changes add logic to preserve the raw request body and ensure it is sent unchanged when proxying requests, which is important for signature verification.
Enhancements for request body handling and HMAC authentication:
sendRequestBodyhelper function is introduced inclientRoutes.tsto send the raw request body buffer when available, falling back to the parsed body for backward compatibility.redirectRequestfunction is updated to usesendRequestBodyfor all HTTP methods that send a body, ensuring the raw bytes are forwarded when possible.Current PR is related to this BGMS PR